Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

C#: Add csharp cors query #18120

Closed
wants to merge 8 commits into from
Closed

C#: Add csharp cors query #18120

wants to merge 8 commits into from

Conversation

Kwstubbs
Copy link
Contributor

Added CORS queries for C# MVC

Fix issues

Change this
Copy link
Contributor

github-actions bot commented Nov 27, 2024

QHelp previews:

csharp/ql/src/experimental/CWE-942/CorsMisconfiguration.qhelp

Credentialed CORS Misconfiguration

A server can send the "Access-Control-Allow-Credentials" CORS header to control when a browser may send user credentials in Cross-Origin HTTP requests.

When the Access-Control-Allow-Credentials header is "true", the Access-Control-Allow-Origin header must have a value different from "*" in order to make browsers accept the header. Therefore, to allow multiple origins for Cross-Origin requests with credentials, the server must dynamically compute the value of the "Access-Control-Allow-Origin" header. Computing this header value from information in the request to the server can therefore potentially allow an attacker to control the origins that the browser sends credentials to.

Recommendation

When the Access-Control-Allow-Credentials header value is "true", a dynamic computation of the Access-Control-Allow-Origin header must involve sanitization if it relies on user-controlled input.

Since the "null" origin is easy to obtain for an attacker, it is never safe to use "null" as the value of the Access-Control-Allow-Origin header when the Access-Control-Allow-Credentials header value is "true".

Example

In the example below, the server allows the browser to send user credentials in a Cross-Origin request. The request header origins controls the allowed origins for such a Cross-Origin request.

using Leaf.Middlewares;
using Microsoft.AspNetCore.Builder;
using Microsoft.AspNetCore.Hosting;
using Microsoft.Extensions.Configuration;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Hosting;

namespace Leaf
{
    public class Startup
    {
        public Startup(IConfiguration configuration)
        {
            Configuration = configuration;
        }

        public IConfiguration Configuration { get; }

        // This method gets called by the runtime. Use this method to add services to the container.
        public void ConfigureServices(IServiceCollection services)
        {
            services.AddControllers();
            //services.AddTransient<MySqlConnection>(_ => new MySqlConnection(Configuration["ConnectionStrings:Default"]));
            services.AddControllersWithViews()
                    .AddNewtonsoftJson(options =>
                        options.SerializerSettings.ReferenceLoopHandling = Newtonsoft.Json.ReferenceLoopHandling.Ignore
            );

            services.AddCors(options => {
                options.AddPolicy("AllowPolicy", builder => builder
                 .WithOrigins("null")
                 .AllowCredentials()
                 .AllowAnyMethod()
                 .AllowAnyHeader());
            });

        }

        // This method gets called by the runtime. Use this method to configure the HTTP request pipeline.
        public void Configure(IApplicationBuilder app, IWebHostEnvironment env)
        {
            app.UseRouting();

            app.UseCors("AllowPolicy");

            app.UseRequestResponseLogging();

            if (env.IsDevelopment())
            {
                app.UseDeveloperExceptionPage();
            }

            app.UseHttpsRedirection();

            app.UseAuthorization();

            app.UseEndpoints(endpoints =>
            {
                endpoints.MapControllers();
            });

        }
    }
}

This is not secure, since an attacker can choose the value of the origin request header to make the browser send credentials to their own server. The use of a allowlist containing allowed origins for the Cross-Origin request fixes the issue:

using Leaf.Middlewares;
using Microsoft.AspNetCore.Builder;
using Microsoft.AspNetCore.Hosting;
using Microsoft.Extensions.Configuration;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Hosting;

namespace Leaf
{
    public class Startup
    {
        public Startup(IConfiguration configuration)
        {
            Configuration = configuration;
        }

        public IConfiguration Configuration { get; }

        // This method gets called by the runtime. Use this method to add services to the container.
        public void ConfigureServices(IServiceCollection services)
        {
            services.AddControllers();
            //services.AddTransient<MySqlConnection>(_ => new MySqlConnection(Configuration["ConnectionStrings:Default"]));
            services.AddControllersWithViews()
                    .AddNewtonsoftJson(options =>
                        options.SerializerSettings.ReferenceLoopHandling = Newtonsoft.Json.ReferenceLoopHandling.Ignore
            );

            services.AddCors(options => {
                options.AddPolicy("AllowPolicy", builder => builder
                 .WithOrigins("http://example.com")
                 .AllowCredentials()
                 .AllowAnyMethod()
                 .AllowAnyHeader());
            });

        }

        // This method gets called by the runtime. Use this method to configure the HTTP request pipeline.
        public void Configure(IApplicationBuilder app, IWebHostEnvironment env)
        {
            app.UseRouting();

            app.UseCors("AllowPolicy");

            app.UseRequestResponseLogging();

            if (env.IsDevelopment())
            {
                app.UseDeveloperExceptionPage();
            }

            app.UseHttpsRedirection();

            app.UseAuthorization();

            app.UseEndpoints(endpoints =>
            {
                endpoints.MapControllers();
            });

        }
    }
}

References

@Kwstubbs
Copy link
Contributor Author

Kwstubbs commented Nov 27, 2024

@github/codeql-csharp Hey team, I am having problems with building the database for the test in this PR. Any recommendations would be great. I get the following error.

[2024-11-26 16:45:06] [build-err] [001] Error:   Compilation error: /Users/kstubbin/Kevin/csharp_cors/csharp/ql/test/experimental/CWE-942/CorsMiconfigurationCredentials.cs(5,1): error CS8805: Program using top-level statements must be an executable.
[2024-11-26 16:45:06] [build-err] [001] Error:   Compilation error: /Users/kstubbin/Kevin/csharp_cors/csharp/ql/test/experimental/CWE-942/CorsMiconfigurationCredentials.cs(9,18): error CS1061: 'IServiceCollection' does not contain a definition for 'AddCors' and no accessible extension method 'AddCors' accepting a first argument of type 'IServiceCollection' could be found (are you missing a using directive or an assembly reference?)

I have wrapped the test in the following class to get rid of the first error:

public class Startup
{
    public void ConfigureServices(IServiceCollection services)
    {

New Errors:

Executing 1 tests in 1 directories.
Extracting test database in /Users/kstubbin/Kevin/csharp_cors/csharp/ql/test/experimental/CWE-942.
[2024-11-26 16:47:21] [build-err] [001] Error:   Compilation error: /Users/kstubbin/Kevin/csharp_cors/csharp/ql/test/experimental/CWE-942/CorsMiconfigurationCredentials.cs(8,35): error CS0246: The type or namespace name 'IServiceCollection' could not be found (are you missing a using directive or an assembly reference?)
[2024-11-26 16:47:21] [build-err] [001] Error:   Compilation error: /Users/kstubbin/Kevin/csharp_cors/csharp/ql/test/experimental/CWE-942/CorsMiconfigurationCredentials.cs(10,44): error CS0103: The name 'args' does not exist in the current context
[2024-11-26 16:47:21] [build-err] [001] Error:   Compilation error: /Users/kstubbin/Kevin/csharp_cors/csharp/ql/test/experimental/CWE-942/CorsMiconfigurationCredentials.cs(14,18): error CS1061: 'IServiceCollection' does not contain a definition for 'AddCors' and no accessible extension method 'AddCors' accepting a first argument of type 'IServiceCollection' could be found (are you missing a using directive or an assembly reference?)
[2024-11-26 16:47:30] [build-err] [024] Error:   Sequence contains more than one element in WebApplication.CreateBuilder(args) at /Users/kstubbin/Kevin/csharp_cors/csharp/ql/test/experimental/CWE-942/CorsMiconfigurationCredentials.cs: (9,14)-(9,48)    at System.Linq.ThrowHelper.ThrowMoreThanOneElementException()
[2024-11-26 16:47:30] [build-err]    at System.Linq.Enumerable.TryGetSingle[TSource](IEnumerable`1 source, Boolean& found)
[2024-11-26 16:47:30] [build-err]    at System.Linq.Enumerable.SingleOrDefault[TSource](IEnumerable`1 source)
[2024-11-26 16:47:30] [build-err]    at Semmle.Extraction.CSharp.Entities.Expressions.Invocation.get_TargetSymbol() in /Users/runner/work/semmle-code/semmle-code/ql/csharp/extractor/Semmle.Extraction.CSharp/Entities/Expressions/Invocation.cs:line 136
[2024-11-26 16:47:30] [build-err]    at Semmle.Extraction.CSharp.Entities.Expressions.Invocation.PopulateExpression(TextWriter trapFile) in /Users/runner/work/semmle-code/semmle-code/ql/csharp/extractor/Semmle.Extraction.CSharp/Entities/Expressions/Invocation.cs:line 37
[2024-11-26 16:47:30] [build-err]    at Semmle.Extraction.CSharp.Entities.Expression`1.<TryPopulate>b__5_0() in /Users/runner/work/semmle-code/semmle-code/ql/csharp/extractor/Semmle.Extraction.CSharp/Entities/Expression`1.cs:line 30
[2024-11-26 16:47:30] [build-err]    at Semmle.Extraction.Context.Try(SyntaxNode node, ISymbol symbol, Action a) in /Users/runner/work/semmle-code/semmle-code/ql/csharp/extractor/Semmle.Extraction/Context.cs:line 467
[2024-11-26 16:47:30] [build-err] [024] Error:   Failed to determine type in builder.Services.AddCors(options =>
[2024-11-26 16:47:30] [build-err] {
[2024-11-26 16:47:30] [build-err]     options.AddPolicy(MyAllowSpecificOrigins,
[2024-11-26 16:47:30] [build-err]                       policy =>
[2024-11-26 16:47:30] [build-err]                       {
[2024-11-26 16:47:30] [build-err]                           policy.SetIsOriginAllowed(test => true).AllowCredentials().AllowAnyHeader().AllowAnyMethod();
[2024-11-26 16:47:30] [build-err]                       });
[2024-11-26 16:47:30] [build-err] }) at /Users/kstubbin/Kevin/csharp_cors/csharp/ql/test/experimental/CWE-942/CorsMiconfigurationCredentials.cs: (13,0)-(20,2)    at Semmle.Extraction.Context.ReportError(InternalError error) in /Users/runner/work/semmle-code/semmle-code/ql/csharp/extractor/Semmle.Extraction/Context.cs:line 412
[2024-11-26 16:47:30] [build-err]    at Semmle.Extraction.Context.ModelError(SyntaxNode node, String msg) in /Users/runner/work/semmle-code/semmle-code/ql/csharp/extractor/Semmle.Extraction/Context.cs:line 424
[2024-11-26 16:47:30] [build-err]    at Semmle.Extraction.CSharp.Entities.ExpressionNodeInfo.get_Type() in /Users/runner/work/semmle-code/semmle-code/ql/csharp/extractor/Semmle.Extraction.CSharp/Entities/ExpressionNodeInfo.cs:line 66
[2024-11-26 16:47:30] [build-err]    at Semmle.Extraction.CSharp.Entities.Expression..ctor(IExpressionInfo info, Boolean shouldPopulate) in /Users/runner/work/semmle-code/semmle-code/ql/csharp/extractor/Semmle.Extraction.CSharp/Entities/Expression.cs:line 27
[2024-11-26 16:47:30] [build-err]    at Semmle.Extraction.CSharp.Entities.Expression`1..ctor(ExpressionNodeInfo info) in /Users/runner/work/semmle-code/semmle-code/ql/csharp/extractor/Semmle.Extraction.CSharp/Entities/Expression`1.cs:line 13
[2024-11-26 16:47:30] [build-err]    at Semmle.Extraction.CSharp.Entities.Expressions.Invocation..ctor(ExpressionNodeInfo info) in /Users/runner/work/semmle-code/semmle-code/ql/csharp/extractor/Semmle.Extraction.CSharp/Entities/Expressions/Invocation.cs:line 14
[2024-11-26 16:47:30] [build-err]    at Semmle.Extraction.CSharp.Entities.Expressions.Invocation.Create(ExpressionNodeInfo info) in /Users/runner/work/semmle-code/semmle-code/ql/csharp/extractor/Semmle.Extraction.CSharp/Entities/Expressions/Invocation.cs:line 21
[2024-11-26 16:47:30] [build-err]    at Semmle.Extraction.CSharp.Entities.Expressions.Factory.Create(ExpressionNodeInfo info) in /Users/runner/work/semmle-code/semmle-code/ql/csharp/extractor/Semmle.Extraction.CSharp/Entities/Expressions/Factory.cs:line 58
[2024-11-26 16:47:30] [build-err]    at Semmle.Extraction.CSharp.Entities.Expressions.ImplicitCast.Create(ExpressionNodeInfo info) in /Users/runner/work/semmle-code/semmle-code/ql/csharp/extractor/Semmle.Extraction.CSharp/Entities/Expressions/ImplicitCast.cs:line 160
[2024-11-26 16:47:30] [build-err]    at Semmle.Extraction.CSharp.Entities.Expression.CreateFromNode(ExpressionNodeInfo info) in /Users/runner/work/semmle-code/semmle-code/ql/csharp/extractor/Semmle.Extraction.CSharp/Entities/Expression.cs:line 106
[2024-11-26 16:47:30] [build-err]    at Semmle.Extraction.CSharp.Entities.Expression.Create(Context cx, ExpressionSyntax node, IExpressionParentEntity parent, Int32 child) in /Users/runner/work/semmle-code/semmle-code/ql/csharp/extractor/Semmle.Extraction.CSharp/Entities/Expression.cs:line 104
[2024-11-26 16:47:30] [build-err]    at Semmle.Extraction.CSharp.Entities.Statements.ExpressionStatement.PopulateStatement(TextWriter trapFile) in /Users/runner/work/semmle-code/semmle-code/ql/csharp/extractor/Semmle.Extraction.CSharp/Entities/Statements/ExpressionStatement.cs:line 21
[2024-11-26 16:47:30] [build-err]    at Semmle.Extraction.CSharp.Entities.Statement.Populate(TextWriter trapFile) in /Users/runner/work/semmle-code/semmle-code/ql/csharp/extractor/Semmle.Extraction.CSharp/Entities/Statement.cs:line 32
[2024-11-26 16:47:30] [build-err]    at Semmle.Extraction.CSharp.Entities.Statement`1.Populate(TextWriter trapFile) in /Users/runner/work/semmle-code/semmle-code/ql/csharp/extractor/Semmle.Extraction.CSharp/Entities/Statement`1.cs:line 31
[2024-11-26 16:47:30] [build-err]    at Semmle.Extraction.FreshEntity.<TryPopulate>b__2_0() in /Users/runner/work/semmle-code/semmle-code/ql/csharp/extractor/Semmle.Extraction/Entities/Base/FreshEntity.cs:line 18
[2024-11-26 16:47:30] [build-err]    at Semmle.Extraction.Context.Try(SyntaxNode node, ISymbol symbol, Action a) in /Users/runner/work/semmle-code/semmle-code/ql/csharp/extractor/Semmle.Extraction/Context.cs:line 467

@michaelnebel
Copy link
Contributor

@github/codeql-csharp Hey team, I am having problems with building the database for the test in this PR. Any recommendations would be great.

Thank you for doing this!
Added some comments on, how you can get the test to compile.

Copy link
Contributor

@michaelnebel michaelnebel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for doing this!
I have added an initial set of comments and clarifying questions.

We are also in the process of moving experimental queries to https://github.com/GitHubSecurityLab/CodeQL-Community-Packs/
Do you want to add the query there instead?

csharp/ql/src/experimental/CWE-942/CorsMisconfiguration.ql Outdated Show resolved Hide resolved
csharp/ql/src/experimental/CWE-942/CorsMisconfiguration.ql Outdated Show resolved Hide resolved
csharp/ql/src/experimental/CWE-942/CorsMisconfiguration.ql Outdated Show resolved Hide resolved
csharp/ql/src/experimental/CWE-942/CorsMisconfiguration.ql Outdated Show resolved Hide resolved
csharp/ql/src/experimental/CWE-942/CorsMisconfiguration.ql Outdated Show resolved Hide resolved
csharp/ql/src/experimental/CWE-942/CorsMisconfiguration.ql Outdated Show resolved Hide resolved
csharp/ql/src/experimental/CWE-942/CorsMisconfiguration.ql Outdated Show resolved Hide resolved
csharp/ql/src/experimental/CWE-942/CorsMisconfiguration.ql Outdated Show resolved Hide resolved
csharp/ql/src/experimental/CWE-942/CorsMisconfiguration.ql Outdated Show resolved Hide resolved
@Kwstubbs
Copy link
Contributor Author

@michaelnebel Lets do the review process here to make sure all the checks are good and then I will open a PR in the Community Pack 😄

Copy link
Contributor

@michaelnebel michaelnebel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you looking into the comments. I have added a second round of comments, which could improve the precision of the query.

csharp/ql/src/experimental/CWE-942/CorsMisconfiguration.ql Outdated Show resolved Hide resolved
csharp/ql/src/experimental/CWE-942/CorsMisconfiguration.ql Outdated Show resolved Hide resolved
csharp/ql/src/experimental/CWE-942/CorsMisconfiguration.ql Outdated Show resolved Hide resolved
/**
* Holds if `c` always returns `true`.
*/
private predicate alwaysReturnsTrue(Callable c) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the PR to the community lab is opened, it would be great, if this code is not copied, but re-used with the implementation where it was copied from.

Copy link
Contributor Author

@Kwstubbs Kwstubbs Dec 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, how do I import a library with in Security Features folder (theres a space in the import?). It just seemed like a "library" of another query so I didn't want my query to be dependent on it if there are changes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spaces in paths are turned into _, so in this case Security_Features.
For code that is shared between queries, I would recommend moving it to the library pack in the CodeQL community pack.

Copy link
Contributor

@michaelnebel michaelnebel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great!
Thank you :-)
If you are interested, you can try and use DCA for testing the new query (this can also be done when it is included in the CodeQL community pack).
Also, once again, I apologise if I went a bit overboard with review suggestions for improvements!

@Kwstubbs
Copy link
Contributor Author

Merged in Community Pack

@Kwstubbs Kwstubbs closed this Dec 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants